feat: support backoff/retry in OTLP #3126
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3126 +/- ##
=======================================
+ Coverage 79.6% 80.2% +0.6%
=======================================
Files 124 128 +4
Lines 23174 22884 -290
=======================================
- Hits 18456 18368 -88
+ Misses 4718 4516 -202 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
scottgerring
force-pushed
the
feat/retry-logic
branch
3 times, most recently
from
db6ed67 to
3847b26
Compare
scottgerring
force-pushed
the
feat/retry-logic
branch
from
3847b26 to
fb141db
Compare
| otel_debug!(name: "TonicLogsClient.ExportFailed", error = &error); | ||
| Err(OTelSdkError::InternalFailure(error)) | ||
| } | ||
| let batch = Arc::new(batch); |
There was a problem hiding this comment.
see if we can avoid this
scottgerring
force-pushed
the
feat/retry-logic
branch
from
603c31e to
75f0d71
Compare
| @@ -35,6 +35,7 @@ tracing = {workspace = true, optional = true} | |||
|
|
|||
| prost = { workspace = true, optional = true } | |||
| tonic = { workspace = true, optional = true } | |||
| tonic-types = { workspace = true, optional = true } | |||
There was a problem hiding this comment.
Needed for gRPC error type
| impl LogExporter for OtlpHttpClient { | ||
| #[cfg(feature = "http-retry")] | ||
| async fn export(&self, batch: LogBatch<'_>) -> OTelSdkResult { | ||
| let policy = RetryPolicy { |
There was a problem hiding this comment.
Should we create a default retry policy and share it between all exporters?
| .map_err(|e| OTelSdkError::InternalFailure(e.message)) | ||
| } | ||
|
|
||
| #[cfg(not(feature = "http-retry"))] |
There was a problem hiding this comment.
This is a massive duplication of code.
If we decide that http export always has retry behaviour, which means we include the unstable runtime feature, then we can remove all of this.
Alternatively we can provide export once, with all the extra pomp and fanfare to support retry, and then just use the stub "don't actually retry" impl. There would be some slight runtime overhead to this, but the codebase would be much simpler.
There was a problem hiding this comment.
Comment above applies to the other HTTP exporters also.
scottgerring
changed the title
This isn't ready for review! Opening PR early for convenience.
Fixes #3081, building on the work started by @AaronRM 🤝
Changes
A new retry module added to
opentelemetry-sdkModels the sorts of retry an operation may request (retry / can't retry / throttle), and provides a helper
retry_with_backoffmechanism that can be used to wrap up a retryable operation and retry it. The helper relies onexperimental_async_runtimefor its runtime abstraction, to provide the actual pausing. It also takes a lambda to classify the error, so the caller can inform the retry mechanism if a retry is required.A new retry_classification module added to
opentelemetry-otlpThis bit takes the actual error responses that we get back over OTLP and maps them back to the retry model. Because this is OTLP-specific stuff it belongs here rather than alongside the retry code.
Retry binding
... happens in each one of the concrete exporters to tie it all together.
Open Questions
experimental_async_runtimealways, like the gRPC exporters do?Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes